internal/ethapi: add eth_SendRawTransactionSync #32830
Conversation
aodhgan
requested review from
fjl,
lightclient,
rjl493456442 and
s1na
as code owners
aodhgan
changed the title
|
We can do a slight refactor with this PR merged: #32697 (comment) |
This PR was merged ^ |
aodhgan
force-pushed
the
ag/feat/add-ethsendRawTransactionSync
branch
from
0daa396 to
3a3c46a
Compare
The given examples (missing nonce or insufficient funds) are already covered by their own more granular codes (https://github.com/aodhgan/go-ethereum/blob/ag/feat/add-ethsendRawTransactionSync/internal/ethapi/errors.go#L102). There is an option to coalesce these into the new error code 5 but dont see much value? (Also considering the EIP says SHOULD (vs MUST)). The "node not ready to accept new transactions" seems to require a deeper set of changes? wdyt @s1na? |
Got it, that makes sense. Since the EIP says SHOULD and not MUST, it’s optional. I just wanted to check if we plan to follow that or keep using the current detailed error codes. |
On the nod ready-ness we could query and see if it is in sync or not, but that's not something we do for the other sendTransaction variants. I'm not sure why it was introduced to this method. On the other point I think I agree that coalescing means losing info, and given the wording, I'd go with the current approach. |
ethclient/ethclient.go
Outdated
| timeout time.Duration, | ||
| ) (*types.Receipt, error) { | ||
| var buf bytes.Buffer | ||
| if err := tx.EncodeRLP(&buf); err != nil { |
There was a problem hiding this comment.
What about naming it SendTransactionSync? and the other one SendRawTransactionSync
There was a problem hiding this comment.
Also I think timeout should be optional
internal/ethapi/api.go
Outdated
|
|
||
| case err, ok := <-subErrCh: | ||
| if !ok || err == nil { | ||
| // subscription closed; disable this case |
There was a problem hiding this comment.
I don't understand why you don't return at this point? am I missing something?
There was a problem hiding this comment.
think that was a stale case from a another implementation i tried. changed in
409eea4
ethclient/ethclient.go
Outdated
| tx *types.Transaction, | ||
| timeout ...time.Duration, |
There was a problem hiding this comment.
that means users can pass in multiple timeout parameters :) I'd make it a pointer and not pass it to the rpc call if it's nil. Or define a new method SendTransactionSyncWithTimeout and SendRawTransactionSyncWithTimeout
internal/ethapi/api.go
Outdated
| } | ||
| } | ||
| // Otherwise, bubble the caller's context error (canceled or deadline). | ||
| if err := ctx.Err(); err != nil { |
There was a problem hiding this comment.
I don't follow why you then go with the caller's context error. Deadlines from the parent context will propagate to receiptCtx also. And receiptCtx generally might "know more" what went wrong.
There was a problem hiding this comment.
Ah I missed the fallback return. Still not clear to me why we need this condition. The receiptCtx should contain an error from its parent ctx
There was a problem hiding this comment.
think I was trying (in vain) to distinguish between different timeouts. Updated to just using the child context error: e203026
6 participants
New RPC method eth_sendRawTransactionSync(rawTx, timeoutMs?) that submits a signed tx and blocks until a receipt is available or a timeout elapses.
Uses a ChainEvent subscription to grep receipts and filter for the receipt of interest.
Two CLI flags to tune server-side limits:
--rpc.txsync.defaulttimeout (default wait window)
--rpc.txsync.maxtimeout (upper bound; requests are clamped)
Success/timeout unit tests added.
closes #32094